Skip to content

Conversation

@kiburtse
Copy link
Contributor

What, How & Why?

☑️ ToDos

  • 📝 Changelog update
  • 🚦 Tests (or not relevant)
  • C-API, if public C++ API changed
  • bindgen/spec.yml, if public C++ API changed

@tgoyne
Copy link
Member

tgoyne commented Feb 14, 2024

I did something similar in #7029, and it turned out to not work. The problem is that UNITTEST_LOG_LEVEL is specifically the log level of the unit test framework itself, so setting UNITTEST_LOG_LEVEL=off results in test failures not printing anything. The log level of the unit test framework and the log level of the logger used in tests needs to be separate environment variables.

The code checking the environment variable should also not be in the main library, as it's purely a testing thing.

@coveralls-official
Copy link

Pull Request Test Coverage Report for Build kirill.burtsev_130

Details

  • 36 of 40 (90.0%) changed or added relevant lines in 5 files are covered.
  • 47 unchanged lines in 10 files lost coverage.
  • Overall coverage increased (+0.01%) to 91.891%

Changes Missing Coverage Covered Lines Changed/Added Lines %
test/object-store/sync/app.cpp 1 2 50.0%
test/test_all.cpp 8 11 72.73%
Files with Coverage Reduction New Missed Lines %
src/realm/util/file_mapper.cpp 1 76.26%
src/realm/array_blobs_big.cpp 2 98.72%
src/realm/query_engine.hpp 2 93.96%
src/realm/query_expression.hpp 2 93.52%
src/realm/uuid.cpp 2 97.01%
src/realm/sync/noinst/protocol_codec.hpp 3 76.64%
src/realm/sync/noinst/client_reset.cpp 4 92.96%
src/realm/util/serializer.cpp 5 90.03%
test/fuzz_group.cpp 11 55.7%
src/realm/sync/noinst/server/server.cpp 15 76.3%
Totals Coverage Status
Change from base Build 2036: 0.01%
Covered Lines: 235529
Relevant Lines: 256314

💛 - Coveralls

Copy link
Contributor

@michael-wb michael-wb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple of comments

endif()
endif()

if(REALM_TEST_LOGGING)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recommend keeping the TEST_ENABLE_LOGGING value so it works like before (based on the logic in test_file.hpp) and update the logic to something like this:

set(logging_level "off")
if(DEFINED REALM_TEST_LOGGING_LEVEL)
    if (NOT "${REALM_TEST_LOGGING_LEVEL}" STREQUAL "")
        set(logging_level ${REALM_TEST_LOGGING_LEVEL})
    endif()
elseif(TEST_ENABLE_LOGGING)
    set(logging_level "all")
endif()
set(REALM_TEST_LOGGING_LEVEL ${logging_level} CACHE STRING "Object-store tests logging level" FORCE)
message(STATUS "Test logging level: ${REALM_TEST_LOGGING_LEVEL}")
target_compile_definitions(ObjectStoreTestLib PRIVATE TEST_LOGGING_LEVEL=${REALM_TEST_LOGGING_LEVEL})

Comment on lines +114 to +127
bool Logger::get_env_log_level_if_set(Level& level) noexcept
{
const char* str = getenv("UNITTEST_LOG_LEVEL");
if (!str || strlen(str) == 0)
return true;

std::istringstream in(str);
in.imbue(std::locale::classic());
in.flags(in.flags() & ~std::ios_base::skipws); // Do not accept white space
in >> level;

return in && in.get() == std::char_traits<char>::eof();
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommend moving this to the test directory (e.g. test/util/) instead of the Core base.

util::Logger::set_default_level_threshold(TestSyncManager::default_log_level());
if (const char* crypt_key = test_util::crypt_key()) {
encryption_key = std::vector<char>(crypt_key, crypt_key + 64);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't forget to address line 200 of this file, since it also uses the TEST_ENABLE_LOGGING variable, which is no longer being set.

#if TEST_ENABLE_LOGGING
                   m_logger = util::Logger::get_default_logger();
#else
                   // Logging is disabled, use a NullLogger to prevent printing anything
                   m_logger.reset(new util::NullLogger());
#endif

@kiburtse kiburtse closed this Aug 13, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants